Pull Request Standards

What is a Pull Request ?

A Pull Request, often abbreviated as PR, is proposed code changes to a codebase, which satisy some given requirements (task, bug, tech debt)

It is a way for developers to discuss and review changes, which allows us to ensure code quality and also to learn from each other.

If we consistently adhered to standards about how Pull Requests are named, what goes into a Pull description. Then, we will have a readable history of the codebase, we have a high undestanding of the proposed code changes and why the changes were needed. This will drastically improve our communication of code change, which will improve our quality to review code changes adn detect incorrect code, which will lead to better code quality with less bugs.

Creating Pull Request

Pull Request Title

Title is one of the most important thing of a Pull Request. When we see all the open pull requests of the app, this is the only thing we see. Also, when we go to look at the history of pull requests, the title is also what we see.

All work should flow from a JIRA Ticket, so these items are critical to have in the pull request title. Include a label and then a short description of the change should be included in the title, so that at a glance, a reviewer gets some idea of what is changed.

Rules:

• Pull Request title should include JIRA ticket number. Make sure to enter the JIRA ticket number exactly as it is in the souce system. The ticket number should be in ALL CAPS.
• After that add a label from below types
  1. Feat - Is for new functionality
  2. Fix - Is for existing functionality
  3. Chore - For a change that doesn't create any new functionality but is needed (e.g., version bump, docs)
  4. We can add more options (like docs, refractor) in future, if its really required.
• After the ticket number, include include one space then a short description should be provided in one sentence. Here you use sentence case. Do not include any other special characters. Note the description may not exactly reflect the JIRA defect titles, as there can be multiple pull requests for each ticket. Note, this is for developers so use the description that makes sense to other developers. You can use technical terms, if they are used in development team.
• Example:
  1. PLATT-10101 - Fix - Popup not closing when tapped on the background
  2. PLATT-16644 - Feat - App update

Pull Request Description

Use your PR description as an opportunity to set the reviewer up for a review. It should give enough information to review the code, even if that person hasn't seen the JIRA tickets.

Description (A short description of the change and why it is needed)

Explain the changes you’ve made and include any notes on what problem it addressed, what capabilities it added. The goal is to bring the reviewer up to speed on the WHY, so that when they look at the code, they have an idea on business goal for the change. Don't write a novel, but also don't write a single sentence (unless the PR is really just that small).

Callouts

If there is any particular environment, test user, etc that would be helpful for the reviewer to know, please include it. Also, if you have made any assumptions about the change, this would be great place to include that. Also, many times in the code review you will get asked WHY you did something, it would be great to include those notes beforehand, it greatly improves the code review experience and decreases the time until a PR can be merged. You can also explain a particular line of code which will be fixed in upcoming PR is a massive time saver for the reviewer.

JIRA Link

Add a clickable link to the JIRA Ticket so that reviewer can get further details about the change.

Screenshots/Recordings (Demonstrate the UI changes, if any)

If the PR has added some UI or modified existing ones, some screenshots or recordings should be included, so that reviewer get context on how the app was changed visually. Adding more details like what file or screen the screenshot is will get more clarity. Adding Before and After Screenshots will do great.

PR Dependencies

If the PR is dependent on another PR, please call it out here. Provide the PR # that the PR is dependent on. It would be great if we can find some way to mention in PR title.

Pull Request Review

As PR creator, you own that PR. Your job is to make sure it is reviewed and merged in proper time. Your work is not done until the PR is merged. Responding to PR feedback is a part of the user story and you should have allocated some time during estimation.

PR Creator Guidelines

• You should respond to all comments and address required fixes as soon as possible.
• An open PR has a higher priority than any other development work that is not yet raised as PR.
• If the reviewer has marked the PR as Request Changes and you have made the changes, you should request a re-review.
• If you make significant changes to a PR after it has been approved, you must request a re-review.

Reviewer Guidelines

• A PR requires atleat 2 approvers, one of the approvers must be a code/module owner. More than 2 approvals is perfectly fine.
• For comments added by reviewer, include code examples of change you suggest so that it is very clear what you feel could be changed in what way.
• If the reviwer has made a comment requesting a code fix and the PR creator has made the fix, the reviewer should make that comment thread as resovled. The reviewer should also mark other comments as resolved if they no longer require further discussion.
• If the PR has more than one round of back and forth between reviewer(s) and developer(s), a review meeting should be setup as soon as possible to discuss the changes and if possible close the fixes during meeitng. This helps PR get merged quickly and prevent long response delays.

Reviewer Comments

In order to make PR code review comments clearer, we can use the following tags to let the PR raiser know what is required inorder to get PR approved and merged. We can use 5 tags that can be copy and pasted into a comment on a PR. These are:

Praise:

Praises highlight something Positive. Try to leave atleast one of these comments per review. Do not leave false praise (which can actually be damaging). Do look for something to sincerely praise.

FYI:

Use this tag to provide additional information to the PR raiser. This label is stricly to provide general information to the PR raiser might find helpful. This label does not have any effect on the reviewer approving or requesting changes on a PR. Also, PR raiser doesn't have to reply on this comment.

Optional Change:

Use this tag to mention an area of code that could be improved. A PR raiser can make this changes in the Open PR or not make the change. The PR raiser may make the changes as part of another PR. The comment must be clear so that PR raiser can make the changes without any futher help OR the comment must include some examples code to get better idea. This tag indicates the reviewer see there is scope of improvement but its not a mandatory change. Its doesn't block PR approval. PR raiser doesn't need to reply to this comment.

Required Change:

Use this tag to point out a code change that must be fixed before the PR can be approved and merged. If you add this tag as a comment, the PR status must also be marked as Request Changes. This tag blocks the PR from being approved and merged. The comment must be clear so that the PR raiser can make the code changes without futher assistance OR the comment must include some code snippets as examples. If the change is complicated then a meeting must be setup to discuss on the code changes in the PR.

Question/Discussion Point:

Use this tag to ask the PR raiser a question about a code change. A reviewer can use this tag to help them gain a better understanding of why a code change was made or why we have implemented a code change in a certain way. The PR raiser must reply to this question and answer the question raised. After the reply, the reviewer must either change the tag from Question/Discussion Point to either Optional Change OR Required Change. If further disussion required, a meeting must be set to discsuss the code change. Following the meeting, update the tag. This tag will prevent a reviewer from giving approval or request changes for the PR until the question asked is answered.

Pull Request Merging

Once the PR has been reviewed and approved, it can be merged. When merging, the Squash Commit should be used. Make sure the commit message is clear so that the git history is useful and clear.

After Pull Request is merged

Delete your branch. You should no longer need that branch, by deleting it will reduce cluttering.